zvfs: improve libc FILE to integer fd abstraction #83386

cfriedt · 2024-12-25T21:49:00Z

Previously, there was an implicit assumption that Zephyr's internal struct fd_entry * was synonymous with FILE * from the C library.

This is generally not the case and aliasing these two distinct types was preventing a fair bit of functionality from Just Working - namely stdio function calls like fgets() and fopen(). The problem count be seen directly when trying to use a function like zvfs_fdopen().

Instead of aliasing the two types, require that all Zephyr C libraries provide

FILE *z_libc_file_alloc(int fd, const char *mode), to allocate and populate the required fields of a FILE object
int z_libc_file_get_fd(FILE *fp), to convert a FILE* object to an integer file descriptor.

For Picolibc and Newlib-based C libraries, these functions set and get the integer file descriptor from a field of the internal FILE object representation.

For the minimal C library, these functions convert between array index and struct fd_entry *.

The function signature for open() in libc-hooks.c was incorrect. Additionally, the ALIAS_OF() macro causes a compile error, because it simplifies the open function signature to int open(), which is a conflicting definition. Use the correct definition to get around compile errors. Signed-off-by: Chris Friedt <[email protected]>

cfriedt · 2024-12-25T23:33:03Z

@tagunil / @evgeniy-paltsev - ARC mwdt might need a similar change. I would be happy to include it here if you have suggestions.

tagunil · 2024-12-25T23:46:53Z

@tagunil / @evgeniy-paltsev - ARC mwdt might need a similar change. I would be happy to include it here if you have suggestions.

I generally like the idea, but I need time to check if/how it would work for our legacy MW libc.

keith-packard

How does z_libc_file_alloc differ from the POSIX fdopen function? How does z_libc_file_get_fd differ from the POSIX fileno function?

cfriedt · 2024-12-26T06:02:14Z

How does z_libc_file_alloc differ from the POSIX fdopen function? How does z_libc_file_get_fd differ from the POSIX fileno function?

Mostly by name. These functions can't depend on POSIX.

cfriedt · 2024-12-26T17:56:43Z

@tejlmand - do you know if there are similar changes necessary for the armclang toolchain? If so, I would be happy to include them here.

keith-packard · 2024-12-26T18:01:58Z

How does z_libc_file_alloc differ from the POSIX fdopen function? How does z_libc_file_get_fd differ from the POSIX fileno function?

Mostly by name. These functions can't depend on POSIX.

So, the picolibc (and newlib) implementations could just be wrappers?

cfriedt · 2024-12-26T22:25:42Z

Mostly by name. These functions can't depend on POSIX.

So, the picolibc (and newlib) implementations could just be wrappers?

What would the picolibc and newlib implementations be wrappers around? Maybe I've misunderstood the question.

keith-packard · 2024-12-27T00:06:56Z

What would the picolibc and newlib implementations be wrappers around? Maybe I've misunderstood the question.

#define _POSIX_C_SOURCE 200809L
#include <stdio.h>

FILE *z_libc_file_alloc(int fd, const char *mode)
{
    return fdopen(fd, mode);
}

int z_libc_file_get_fd(FILE *f)
{
    return fileno(f);
}

Alternatively, just add fdopen and fileno to the Zephyr API and use them directly.

cfriedt · 2024-12-27T00:25:50Z

What would the picolibc and newlib implementations be wrappers around? Maybe I've misunderstood the question.
#define _POSIX_C_SOURCE 200809L
#include <stdio.h>

FILE *z_libc_file_alloc(int fd, const char *mode)
{
    return fdopen(fd, mode);
}

int z_libc_file_get_fd(FILE *f)
{
    return fileno(f);
}
Alternatively, just add fdopen and fileno to the Zephyr API and use them directly.

That would introduce a POSIX dependency (as well as a dependency cycle), which is what we're trying to avoid in this situation.

It could maybe work if there were similar libc-internal functions that were not aliases of fdopen() or fileno().

In other words, here, we are limited to ISO C only and non-POSIX functions.

keith-packard · 2024-12-27T00:43:19Z

In other words, here, we are limited to ISO C only and non-POSIX functions.

So all we need to do is expose fileno and fdopen as Zephyr functions from the C library, just as we do for other Zephyr APIs which came from POSIX, like strnlen and strtok_r? For newlib, we could use the same hack used for those? Zero code added to Zephyr seems a lot better than copying code from an external library repository.

cfriedt · 2024-12-27T11:45:23Z

In other words, here, we are limited to ISO C only and non-POSIX functions.

So all we need to do is expose fileno and fdopen as Zephyr functions from the C library, just as we do for other Zephyr APIs which came from POSIX, like strnlen and strtok_r?

Not exactly - strnlen() and strtok_r() have zero side-effects and do not deal with management or validation of kernel resources.

Some of the kernel side of this might eventually hop the fence to syscall neighbourhood, and it seems like there would be some dependency cycles formed by calling userspace-facing APIs. From kernel space.

For newlib, we could use the same hack used for those? Zero code added to Zephyr seems a lot better than copying code from an external library repository.

I realize it's tempting to hack things, blur the lines and take shortcuts, for the sake of size-optimization, but I feel it's the wrong approach. Especially because "Zephyr is not a POSIX OS".

I think it's ok to call functions here that are not directly part of POSIX APIs though, so maybe if there are aliases that exist, those can be used instead (even though it's technically still the same code).

Here is a question: if, for example, only _write_r() and _write() are referenced directly, does the write() symbol get discarded if write() is an alias of _write()?

Define fcntl.h constants in terms of zvfs constants. Signed-off-by: Chris Friedt <[email protected]>

cfriedt · 2024-12-28T13:48:51Z

I tried using the aliases as well as the POSIX symbols here, but it unfortunately introduced an infinite recursion. I also tried using the option TC_PROVIDES_POSIX_DEVICE_IO and there were unresolved symbols.

Probably the existing change should suffice, for now.

Previously, there was an implicit assumption that Zephyr's internal struct fd_entry * was synonymous with FILE * from the C library. This is generally not the case and aliasing these two distinct types was preventing a fair bit of functionality from Just Working - namely stdio function calls like fgets() and fopen(). The problem count be seen directly when trying to use a function like zvfs_fdopen(). Instead of aliasing the two types, require that all Zephyr C libraries provide 1. FILE *z_libc_file_alloc(int fd, const char *mode) Allocate and populate the required fields of a FILE object 2. int z_libc_file_get_fd(FILE *fp) Convert a FILE* object to an integer file descriptor. For Picolibc and Newlib-based C libraries, these functions set and get the integer file descriptor from a field of the internal FILE object representation. For the minimal C library, these functions convert between array index and struct fd_entry pointers. Signed-off-by: Chris Friedt <[email protected]>

cfriedt · 2024-12-28T14:08:12Z

fixed assignment of reentrant function pointers in the absence of consistent typing.

cfriedt · 2024-12-31T11:02:45Z

lib/os/fdtable.c

@@ -25,6 +25,11 @@
 #include <zephyr/internal/syscall_handler.h>
 #include <zephyr/sys/atomic.h>

+#ifndef CONFIG_MINIMAL_LIBC
+extern FILE *z_libc_file_alloc(int fd, const char *mode);


Is a better name here z_libc_fd_get_file() to highlight that it's the opposite of z_libc_file_get_fd()?

Naming things is hard, and it's really just an artifact of picolibc and newlib that they use calloc and malloc internally.

In theory, it should be easy enough to create a statically allocated table of FILE objects and have them use that instead of dynamic allocation.

I was looking through my local repo and found an implementation of fopen for Zephyr which doesn't use the underlying POSIX APIs at all. With your re-implementation of z_libc_file_get_file (nee fdopen), I wonder if we shouldn't provide both of these within Zephyr rather than within picolibc; we could then use a static array of FILE objects for both and you'd be able to easily translate between them.

The picolibc stdio implementation provides stable API which can support this, so it's not even a hack :-)

Check out https://github.com/keith-packard/zephyr/blob/picolibc-fopen/lib/libc/picolibc/fopen.c

I was looking through my local repo and found an implementation of fopen for Zephyr which doesn't use the underlying POSIX APIs at all.

That sounds promising.

With your re-implementation of z_libc_fd_get_file (nee fdopen), I wonder if we shouldn't provide both of these within Zephyr rather than within picolibc

That's sort of where I was heading as well. It's simple enough to implement inside of Zephyr and doesn't require any libc modifications.

It's also reasonably easy for other C libraries to do something similar, even those without POSIX support, like IAR.

The main difference between fdopen() and z_libc_fd_get_file() is that they have slightly different use-cases; the former is for calling from user code into the OS and is dependent on the POSIX API, whereas the latter is for calling into the libc from the OS, and doesn't depend on the POSIX API. So they're kind of going in opposite directions. The latter also eliminates possibly recursive calls.

we could then use a static array of FILE objects for both and you'd be able to easily translate between them.

That sounds great too - some Zephyr users prefer to avoid malloc-related things entirely. So that would be a bit more "user friendly" (for certain kinds of users).

The picolibc stdio implementation provides stable API which can support this, so it's not even a hack :-)

👍

@keith-packard - do you want to make a separate PR, or would you prefer if I added e.g. static FILE table here?

I'm happy to rename things as needed as well.

Some might prefer e.g. a __-prefix

The main difference between fdopen() and z_libc_fd_get_file() is that they have slightly different use-cases; the former is for calling from user code into the OS and is dependent on the POSIX API, whereas the latter is for calling into the libc from the OS, and doesn't depend on the POSIX API. So they're kind of going in opposite directions. The latter also eliminates possibly recursive calls.

There's nothing POSIX-specific about fdopen. On newlib, it doesn't use POSIX apis, it uses the existing
wrappers just like fopen does. With picolibc, it would be implemented entirely in Zephyr using non-POSIX Zephyr APIs. Using the POSIX name makes it far easier for developers to use, as long as the semantics are effectively the same. Creating a new name just makes this function harder to find.

That sounds great too - some Zephyr users prefer to avoid malloc-related things entirely. So that would be a bit more "user friendly" (for certain kinds of users).

Yup, I avoid using malloc in most of my embedded code too -- I find it far easier to reason about memory usage when it's all pre-allocated. However, in this case it sure looks like you're replacing one general allocator (malloc) with a special-purpose allocator (static array of FILE structs). I think the argument for doing that isn't very strong; you still need to evaluate the whole application to ensure there won't be any resource starvation.

I prefer to use APIs which don't rely on any dynamic allocation. Picolibc does this by allowing applications to declare FILE structs themselves, rather than calling a function to allocate them. The only problem with this approach is that stdio doesn't generally know about all of the in-use FILE structs, so fflush(NULL) doesn't work. We'd need some helpers to make that easy to use with Zephyr.

That's secondary to how fopen (and "fdopen") should be implemented in Zephyr; I think what we probably want is to implement "fdopen" (whatever the name ends up being) using the fdtable APIs and then implement fopen on top of that, just as picolibc does when using POSIX apis.

There's nothing POSIX-specific about fdopen.

Other than the fact that fdopen is entirely POSIX specific (it's not a part of ISO C).

With picolibc, it would be implemented entirely in Zephyr using non-POSIX Zephyr APIs.

That would be ideal, because then ISO C and POSIX would use the same underlying APIs.

Using the POSIX name makes it far easier for developers to use, as long as the semantics are effectively the same. Creating a new name just makes this function harder to find.

Uhm... I think you're a bit off there.

The point is that there is no dependency cycle. The OS shouldn't refer to a FILE <-> fd mapping managed by the C library if it uses the same name that application code uses. This is because that would be crossing the user-kernel API line twice, adding an API dependency cycle (which is a slippery slope). It's much better to have no cycles in the DAG of API calls.

By having a separate name for the kernel and for users, we mitigate the dependency cycle with a mutual dependency.

It's pretty simple. We will still offer both the POSIX API call and the native Zephyr call.

However, in this case it sure looks like you're replacing one general allocator (malloc) with a special-purpose allocator (static array of FILE structs).

A pool of objects is kind of like a special-purpose allocator, yes, but it's generally considered safer for certain applications than using heap allocation.

The main point is enabling standards-compliant code.

I think the argument for doing that isn't very strong; you still need to evaluate the whole application to ensure there won't be any resource starvation.

Technically, we could do declarative allocation as long as we can put FILE objects into the same linker section. Then it's effectively using both declarative allocation as well as pool-based allocation.

I'd like to eventually convert "files" to be proper k_objects soon. Likely that would require some linkage between the libc FILE and an new kernel object.

I prefer to use APIs which don't rely on any dynamic allocation.

That makes sense for most high-reliability / safety critical applications.

That's secondary to how fopen (and "fdopen") should be implemented in Zephyr

Yes.

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch from d69d5a7 to de629fa Compare December 25, 2024 21:51

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch from de629fa to 6733b5d Compare December 25, 2024 22:05

cfriedt requested review from keith-packard, ycsin, stephanosio, tagunil and evgeniy-paltsev December 25, 2024 23:31

keith-packard reviewed Dec 26, 2024

View reviewed changes

cfriedt requested a review from tejlmand December 26, 2024 17:56

posix: fcntl.h: define constants in terms of zvfs constants

0fa4f0d

Define fcntl.h constants in terms of zvfs constants. Signed-off-by: Chris Friedt <[email protected]>

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch 2 times, most recently from 52c48ca to b1685eb Compare December 28, 2024 13:16

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch from b1685eb to f1bdef2 Compare December 28, 2024 14:07

cfriedt commented Dec 31, 2024

View reviewed changes

cfriedt mentioned this pull request Jan 1, 2025

fix many things #83368

Draft

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zvfs: improve libc FILE to integer fd abstraction #83386

zvfs: improve libc FILE to integer fd abstraction #83386

cfriedt commented Dec 25, 2024 •

edited

Loading

cfriedt commented Dec 25, 2024

tagunil commented Dec 25, 2024

keith-packard left a comment

cfriedt commented Dec 26, 2024

cfriedt commented Dec 26, 2024

keith-packard commented Dec 26, 2024

cfriedt commented Dec 26, 2024

keith-packard commented Dec 27, 2024

cfriedt commented Dec 27, 2024

keith-packard commented Dec 27, 2024

cfriedt commented Dec 27, 2024

cfriedt commented Dec 28, 2024

cfriedt commented Dec 28, 2024

cfriedt Dec 31, 2024 •

edited

Loading

keith-packard Dec 31, 2024 •

edited

Loading

cfriedt Dec 31, 2024

cfriedt Dec 31, 2024

keith-packard Dec 31, 2024

cfriedt Dec 31, 2024 •

edited

Loading

zvfs: improve libc FILE to integer fd abstraction #83386

Are you sure you want to change the base?

zvfs: improve libc FILE to integer fd abstraction #83386

Conversation

cfriedt commented Dec 25, 2024 • edited Loading

cfriedt commented Dec 25, 2024

tagunil commented Dec 25, 2024

keith-packard left a comment

Choose a reason for hiding this comment

cfriedt commented Dec 26, 2024

cfriedt commented Dec 26, 2024

keith-packard commented Dec 26, 2024

cfriedt commented Dec 26, 2024

keith-packard commented Dec 27, 2024

cfriedt commented Dec 27, 2024

keith-packard commented Dec 27, 2024

cfriedt commented Dec 27, 2024

cfriedt commented Dec 28, 2024

cfriedt commented Dec 28, 2024

cfriedt Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

keith-packard Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt Dec 31, 2024

Choose a reason for hiding this comment

cfriedt Dec 31, 2024

Choose a reason for hiding this comment

keith-packard Dec 31, 2024

Choose a reason for hiding this comment

cfriedt Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt commented Dec 25, 2024 •

edited

Loading

cfriedt Dec 31, 2024 •

edited

Loading

keith-packard Dec 31, 2024 •

edited

Loading

cfriedt Dec 31, 2024 •

edited

Loading